Skip to content

feat(sdk/subagent): condenser for subagents#2349

Open
VascoSch92 wants to merge 1 commit intomainfrom
vasco/subagent-condenser
Open

feat(sdk/subagent): condenser for subagents#2349
VascoSch92 wants to merge 1 commit intomainfrom
vasco/subagent-condenser

Conversation

@VascoSch92
Copy link
Contributor

@VascoSch92 VascoSch92 commented Mar 6, 2026

Summary

(ref #2186 )

Allow subagents defined via Markdown files to declare a condenser configuration. The condenser LLM is inherited from the agent's own LLM at factory time — the definition only stores the tuning parameters (e.g., max_size, keep_first).

Changes

openhands-sdk/openhands/sdk/subagent/schema.py

  • Added condenser: dict[str, Any] | None field to AgentDefinition
  • Added _extract_condenser() with key validation against _CONDENSER_VALID_KEYS
  • Unknown keys produce a clear error listing valid parameters

openhands-sdk/openhands/sdk/subagent/registry.py

  • In agent_definition_to_factory, constructs an LLMSummarizingCondenser from the config when present, inheriting the agent's LLM with usage_id: "condenser"

tests/sdk/subagent/test_subagent_schema.py

  • 8 new tests: default None, dict construction, YAML frontmatter loading, metadata exclusion, unknown key rejection, non-dict value rejection, absent field, and a sync test that verifies _CONDENSER_VALID_KEYS
    matches LLMSummarizingCondenser.model_fields to prevent drift

Usage

  ---
  name: long-running-agent
  condenser:
    max_size: 100
    keep_first: 3
  ---

  You are an agent that handles long tasks.

Design decisions

  • Config-only in definition, object in factory: The AgentDefinition stores a plain dict since the condenser needs an LLM instance which is only available at factory time. This mirrors how model: inherit works.
  • Key validation at parse time: Unknown keys are rejected early during load() rather than deferring to Pydantic at construction time, giving clearer errors tied to the source file.
  • Value validation deferred: Value types (e.g., max_size must be int > 0) are validated by Pydantic when LLMSummarizingCondenser is constructed — no need to duplicate those constraints.
  • Sync test guards against drift: test_condenser_valid_keys_match_llm_summarizing_condenser will fail if LLMSummarizingCondenser fields change without updating _CONDENSER_VALID_KEYS.

Checklist

  • If the PR is changing/adding functionality, are there tests to reflect this?
  • If there is an example, have you run the example to make sure that it works?
  • If there are instructions on how to run the code, have you followed the instructions and made sure that it works?
  • If the feature is significant enough to require documentation, is there a PR open on the OpenHands/docs repository with the same branch name?
  • Is the github CI passing?

Agent Server images for this PR

GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server

Variants & Base Images

Variant Architectures Base Image Docs / Tags
java amd64, arm64 eclipse-temurin:17-jdk Link
python amd64, arm64 nikolaik/python-nodejs:python3.12-nodejs22 Link
golang amd64, arm64 golang:1.21-bookworm Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:37c2787-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-37c2787-python \
  ghcr.io/openhands/agent-server:37c2787-python

All tags pushed for this build

ghcr.io/openhands/agent-server:37c2787-golang-amd64
ghcr.io/openhands/agent-server:37c2787-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:37c2787-golang-arm64
ghcr.io/openhands/agent-server:37c2787-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:37c2787-java-amd64
ghcr.io/openhands/agent-server:37c2787-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:37c2787-java-arm64
ghcr.io/openhands/agent-server:37c2787-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:37c2787-python-amd64
ghcr.io/openhands/agent-server:37c2787-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-amd64
ghcr.io/openhands/agent-server:37c2787-python-arm64
ghcr.io/openhands/agent-server:37c2787-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-arm64
ghcr.io/openhands/agent-server:37c2787-golang
ghcr.io/openhands/agent-server:37c2787-java
ghcr.io/openhands/agent-server:37c2787-python

About Multi-Architecture Support

  • Each variant tag (e.g., 37c2787-python) is a multi-arch manifest supporting both amd64 and arm64
  • Docker automatically pulls the correct architecture for your platform
  • Individual architecture tags (e.g., 37c2787-python-amd64) are also available if needed

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

API breakage checks (Griffe)

Result: Passed

Action log

@VascoSch92 VascoSch92 requested a review from all-hands-bot March 6, 2026 14:54
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Agent server REST API breakage checks (OpenAPI)

Result: Failed

Log excerpt (first 1000 characters)
{"asctime": "2026-03-06 14:54:26,430", "levelname": "WARNING", "name": "openhands.agent_server.config", "filename": "config.py", "lineno": 173, "message": "\u26a0\ufe0f OH_SECRET_KEY was not defined. Secrets will not be persisted between restarts."}
::error title=openhands-agent-server REST API::Breaking REST API change detected without MINOR version bump (1.12.0 -> 1.12.0).

Breaking REST API changes detected compared to baseline release:
- the 'file' request property type/format changed from 'string'/'' to 'string'/'binary'
/home/runner/work/software-agent-sdk/software-agent-sdk/.venv/lib/python3.13/site-packages/litellm/llms/custom_httpx/async_client_cleanup.py:66: DeprecationWarning: There is no current event loop
  loop = asyncio.get_event_loop()

Action log

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands-sdk/openhands/sdk/subagent
   registry.py126893%116, 241–242, 246, 251, 253, 295–296
   schema.py88495%44, 57, 97, 113
TOTAL19634573470% 

Copy link
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Taste Rating: Acceptable

Core design is clean and pragmatic, but this PR falls into eval-risk territory and has one style issue to address.


🔴 Eval Risk - Human Review Required

This PR modifies condenser/memory behavior, which can affect benchmark performance. Per repo policy, changes to memory/condenser behavior require human maintainer review + lightweight evals before approval.

Recommendation: Flag for maintainer review and consider running lightweight evals to verify no regression in agent performance.


Key Design Strengths

Clean separation: Config (dict) in definition, object at factory time - mirrors the model: inherit pattern
Smart validation: Keys validated at parse time (clear errors), values validated by Pydantic (no duplication)
Strong tests: Real behavior tested, sync test prevents drift with LLMSummarizingCondenser.model_fields
No breaking changes: Additive feature with sensible defaults


See inline comments for the style issue.

@VascoSch92 VascoSch92 marked this pull request as ready for review March 6, 2026 15:00
Copy link
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Acceptable - Clean design, well-tested, pragmatic approach.

Eval-Risk Confirmation: This modifies condenser/memory behavior. Per repo policy, requires human maintainer review + lightweight evals before merge.

Code Quality: Solid implementation with comprehensive test coverage. The design choice to store config as dict[str, Any] in AgentDefinition and construct the condenser at factory time mirrors the model: inherit pattern and is a reasonable approach. The sync test guarding against drift between _CONDENSER_VALID_KEYS and LLMSummarizingCondenser is a pragmatic safeguard.

No blocking issues found. ✅ Code is mergeable once eval verification confirms no benchmark regressions.

from openhands.sdk.context.condenser import LLMSummarizingCondenser

condenser = LLMSummarizingCondenser(
llm=llm.model_copy(update={"usage_id": "condenser"}),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do worry a little bit that the more of these we do the more we'll have to sort out... what if we sent a profile name, or think about how this could be like to replace these LLMs with profiles references if that makes sense?

Though the condenser is a pretty obvious place, maybe not exactly to worry much about

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah solution would be to save a condenser LLM profile that we use just for that and we go on the default one if there is no one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! That sounds great to me, or even, if possible, read the condenser profile at the start of the conversation, if none initialize one from the other

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I think this should be addressed in another PR, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, the more model_copy(update...) we add to the codebase, the more we add tech debt and need to fix it

But I also see the opposite argument here. I'm just starting to feel we're adding too many, though I could be wrong

@all-hands-bot
Copy link
Collaborator

[Automatic Post]: It has been a while since there was any activity on this PR. @VascoSch92, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants